-
Notifications
You must be signed in to change notification settings - Fork 391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pump Version python to 3.12, 3.13. Allow pip install for python 3.12, 3.13 #210
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
It's strange, I didn't make any changes to the Python code, yet it's causing CI errors with mypy. |
@maycuatroi |
- Improve CI libraries
@odashi -san. I fixed CI to the last version of mypy and black. Please approve the CI scan |
@maycuatroi Thanks! I think you tweaked some code blocked by version switches, e.g.,
These blocks are activated while the library is running on a conditioned version of Python, and the code block must follow the correct syntax on the corresponding version. I guessed that this is a good time to drop support for legacy (EOL-ed) versions such as 3.7 and 3.8, by simply removing corresponding blocks from the library. |
- Remove ORL python 3.8, 3.9 - Fix CI unit-test
@odashi -san
I really enjoy using this library, and I appreciate your professionalism. To ensure everything works across multiple Python versions, I also created a custom Docker Compose setup for running the CI tests on various versions 😄 (I'm not commit it yet): version: "3.8"
services:
unit-tests:
image: python:3.13
environment:
- PYTHONPATH=src
volumes:
- .:/app # This will copy all files in the current directory into the /app directory in the container
working_dir: /app
command: >
bash -c "
python -m pip install --upgrade pip &&
python -m pip install -e '.[dev]' &&
python -m pytest src
"
black:
image: python:3.13
volumes:
- .:/app # Mounting the project directory
working_dir: /app
command: >
bash -c "
python -m pip install --upgrade pip &&
python -m pip install black &&
python -m black -v --check src
"
flake8:
image: python:3.13
volumes:
- .:/app # Mounting the project directory
working_dir: /app
command: >
bash -c "
python -m pip install --upgrade pip &&
python -m pip install pyproject-flake8 &&
pflake8 -v src
"
isort:
image: python:3.13
volumes:
- .:/app # Mounting the project directory
working_dir: /app
command: >
bash -c "
python -m pip install --upgrade pip &&
python -m pip install isort &&
python -m isort --check src
"
mypy:
image: python:3.13
volumes:
- .:/app # Mounting the project directory
working_dir: /app
command: >
bash -c "
python -m pip install --upgrade pip &&
python -m pip install '.[mypy]' &&
python -m mypy src
" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Co-authored-by: Yusuke Oda <[email protected]>
…python 3.12 - Fix CI for python 3.11.
@odashi -san |
@odashi -san. I think things look good 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, basically everything looks good. Comments are almost based on coding conventions.
Co-authored-by: Yusuke Oda <[email protected]>
@odashi -san, please approve the workflow, I fixed the isort issue 😄 |
@odashi -san, please help me review. I updated follow your comment 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost done I think, but there are still some little comments (maybe these are the last ones)
src/latexify/ast_utils_test.py
Outdated
(ast.Constant(value=None), True), | ||
(ast.Constant(value=123), True), | ||
(ast.Constant(value="baz"), True), | ||
(ast.Expr(value=ast.Constant(value=456)), False), | ||
(ast.Global(names=["qux"]), False), | ||
], | ||
) | ||
def test_is_constant_legacy(value: ast.AST, expected: bool) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks there exists some test cases from that only @test_utils.require_at_most(7)
was removed like this. Could you check the changes entirely and remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decorator (@test_utils.require_at_most(7)) was previously used to indicate that the test case would run on Python versions >= 3.7. So, even when we were on Python 3.9, the test case was still executed. Now that we've upgraded to at least Python 3.9, the test case should still run as intended. I believe it’s best to keep the test case as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require_at_most(k)
requires versions equal or less than 3.k. the decorated function does nothing (skip everything) otherwise.
latexify_py/src/latexify/test_utils.py
Lines 51 to 52 in 678cd0e
if sys.version_info.minor > minor: | |
return |
name, | ||
args, | ||
body, | ||
decorator_list, | ||
returns=None, | ||
type_comment=None, | ||
type_params=None, | ||
lineno=None, | ||
col_offset=None, | ||
end_lineno=None, | ||
end_col_offset=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add type hints.
src/latexify/ast_utils_test.py
Outdated
(ast.Constant(value=None), False), | ||
(ast.Constant(value=123), False), | ||
(ast.Constant(value="baz"), True), | ||
(ast.Expr(value=ast.Constant(value=456)), False), | ||
(ast.Global(names=["qux"]), False), | ||
], | ||
) | ||
def test_is_str_legacy(value: ast.AST, expected: bool) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test case as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with decorator @test_utils.require_at_most(7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removing every test function with decoration @test_utils.require_at_most(7)
should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odashi Yes, I removed all the test function decorated with @test_utils.require_at_most(7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odashi please check and approve the Checks :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Overview
Update file
pyproject.toml
to enable pip install for python 3.12References
Example workbook work perfect when I pip install on python 3.12